prospector: cache resolved glob patterns during init #4269
Conversation
| @@ -42,12 +43,33 @@ func NewLog(p *Prospector) (*Log, error) { | |||
| return prospectorer, nil | |||
| } | |||
|
|
|||
| func (l *Log) resolvePaths() error { | |||
| if l.paths != nil { | |||
There was a problem hiding this comment.
I would avoid making a distinction between non-nil and zero length slices. This can lead to subtle programming mistakes. I would check for len(l.paths) > 0
There was a problem hiding this comment.
thanks, yeah I prefer that too
| l.paths = l.config.Paths | ||
| return nil | ||
| } | ||
| l.paths = []string{} |
There was a problem hiding this comment.
This is unnecessary if you check the len() of the slice above, rather than for non-nil.
|
@7AC I didn't check the PR yet but I strongly recommend to add some system test to it to check if everything goes well during restart. I remember doing the first implementation without the "extra glob" was a bit tricky, especially on windows. We must make sure the right paths are picked up again. |
ruflin
added
Filebeat
filebeat/prospector/config.go
Outdated
|
|
||
| func (config *prospectorConfig) resolvePaths() error { | ||
| var paths []string | ||
| if !config.recursiveGlob { |
There was a problem hiding this comment.
The reason this is not working is that recursiveGlob is a private variable and golang can only extract values in global variables. You must set it to RecursiveGlob in prospectorConfig.
| name="output contains 'entry1'") | ||
|
|
||
| # Wait a momemt to make sure registry is completely written | ||
| time.sleep(1) |
There was a problem hiding this comment.
Sleep is very flaky with these kind of things. We normally rely on the log_contains messages here. Check some other tests for it.
There was a problem hiding this comment.
It's copy/paste from test_registrar.py (had the same observation btw :) I don't think I need it here anyways I'll remove it
| @@ -1396,3 +1398,8 @@ def test_registrar_files_with_prospector_level_processors(self): | |||
| "inode": stat.st_ino, | |||
| "device": stat.st_dev, | |||
| }, file_state_os) | |||
|
|
|||
|
|
|||
| if __name__ == '__main__': | |||
There was a problem hiding this comment.
With this addition, can you directly "run" the file without prepending it with nosetests?
There was a problem hiding this comment.
yep, just need to pip install a couple of things
| @@ -350,6 +353,16 @@ def output_has(self, lines, output_file=None): | |||
| except IOError: | |||
| return False | |||
|
|
|||
| def output_has_message(self, message, output_file=None): | |||
There was a problem hiding this comment.
That will become handy in other tests too.
There was a problem hiding this comment.
Yeah, I tried working with the existing APIs but checking the length wasn't enough since when I restart I want to check for the different message to show up (length still 1)
| lambda: self.output_has_message("entry2"), | ||
| max_timeout=10, | ||
| name="output contains 'entry2'") | ||
|
|
There was a problem hiding this comment.
filebeat must be stopped again here.
CHANGELOG.asciidoc
Outdated
| @@ -31,6 +31,7 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha1...master[Check the HEAD d | |||
| - Fix importing the dashboards when the limit for max open files is too low. {issue}4244[4244] | |||
|
|
|||
| *Filebeat* | |||
| - Make recursive glob resolution work across restarts. {pull}4269[4269] | |||
There was a problem hiding this comment.
Should this be "make recursive glob working"? Not sure it was working before?
Before this change recursive glob patterns were resolved only at runtime, so everytime filebeat restarted it would try to recover its previous state without expanding the patterns first. With this change we expand the patterns once and for all during init, fixing both the start and the restart case.
Resolves #4182